Add support to SSL_CTX_set_keylog_callback() #536

cdelafuente-r7 · 2022-08-29T18:33:55Z

This adds support to OpenSSL's SSL_CTX_set_keylog_callback(). This callback is invoked when TLS key material is generated or received, in order to allow applications to store this keying material for debugging purposes. It is invoked with an SSLSocket and a string containing the key material in the format used by NSS for its SSLKEYLOGFILE debugging output.

This PR adds the Ruby binding keylog_cb and the related tests.

Note that It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements SSL_CTX_set_keylog_callback() from v3.4.2, it does nothing (see libressl/openbsd@648d39f)

Example

    context = OpenSSL::SSL::SSLContext.new
    context.keylog_cb = proc do |ary|
      _sock, line = ary
      File.open('ssl_keylog_file', "a") do |f|
        f.write("#{line}\n")
      end
    end

    tcp_socket = TCPSocket.new @host, @port
    ssl_client = OpenSSL::SSL::SSLSocket.new tcp_socket, context
    ssl_client.sync_close = true
    ssl_client.connect

    ssl_client.puts "hello server!"
    puts ssl_client.gets

    ssl_client.close

cdelafuente-r7 · 2022-08-29T18:35:59Z

ext/openssl/ossl_ssl.c

+    }
+}
+#else
+#define ossl_sslctx_keylog_cb rb_f_notimplement


I'm not sure if this is required since the call to SSL_CTX_set_keylog_callback() (line 965) is also surrounded by #if #endif.

This is unnecessary

rhenium · 2022-08-31T14:02:49Z

ext/openssl/ossl_ssl.c

+
+    ary = rb_ary_new2(2);
+    rb_ary_push(ary, ssl_obj);
+	rb_ary_push(ary, rb_str_new2((const char *)line));


rb_ary_new2() and rb_str_new2() (which BTW are renamed to rb_ary_new_capa() and rb_str_new_cstr() respectively) are unsafe to use here because they can potentially raise a Ruby exception; anything that can raise an exception should be called within rb_protect().

I realized existing code does this. They have to be updated...

Thanks for the heads up! It should be addressed in 9f4fa70

rhenium · 2022-08-31T14:05:23Z

ext/openssl/ossl_ssl.c

+    }
+}
+#else
+#define ossl_sslctx_keylog_cb rb_f_notimplement


This is unnecessary

rhenium · 2022-08-31T14:07:43Z

ext/openssl/ossl_ssl.c

+
+    rb_protect(ossl_call_keylog_cb, ary, &state);
+    if (state) {
+    }


Looks incomplete? You can use ID_callback_state and the exception will be re-raised later (in ossl_start_ssl()).

Thanks! I used ID_callback_state as suggested in 9f4fa70

rhenium · 2022-08-31T14:07:51Z

test/openssl/test_ssl.rb

+      context.keylog_cb = proc do |ary|
+        cb_called = true
+        _sock, line = ary
+        assert_equal(line.split.first, prefix)


assert_equal(expected, actual)

Fixed in 9f4fa70

rhenium · 2022-08-31T14:11:16Z

test/openssl/test_ssl.rb

+      OpenSSL::SSL::TLS1_VERSION,
+      OpenSSL::SSL::TLS1_1_VERSION,
+      OpenSSL::SSL::TLS1_2_VERSION
+    ]


I think testing TLS 1.2 and TLS 1.3 is sufficient. It can make the test code cleaner (no need for ignore_listener_error: true or rescue clauses).

In ruby/openssl's tests, we make the implicit assumption that TLS 1.2 is always supported. You can use tls13_supported? helper method to see if TLS 1.3 is available or not.

Thanks! I refactored the tests in 9f4fa70

rhenium · 2022-08-31T14:14:04Z

This adds support to OpenSSL's SSL_CTX_set_keylog_callback().

Thank you for the patch! That would be nice to have.

cdelafuente-r7 · 2022-09-06T10:50:09Z

Thank you for your review. I'll address everything soon, I'm a bit I'm tied up right now.

cdelafuente-r7 · 2022-09-14T11:18:30Z

ext/openssl/ossl_ssl.c

+static VALUE
+rb_str_new_cstr_stub(VALUE str)
+{
+    return rb_str_new_cstr((const char *)str);
+}


I had to use a stub to avoid some compilation warnings when using rb_protect(). Apparently, the first argument has to be a pointer to a function accepting an argument of type VALUE and using rb_str_new_cstr() directly was an issue:

VALUE rb_protect(VALUE (*)(VALUE), VALUE, int*);

Please, let me know if there is better way to do this.

ext/openssl/ossl_ssl.c

rhenium · 2022-09-14T13:44:25Z

ext/openssl/ossl_ssl.c

+ * It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements
+ * SSL_CTX_set_keylog_callback() from v3.4.2, it does nothing (see
+ * https://github.com/libressl-portable/openbsd/commit/648d39f0f035835d0653342d139883b9661e9cb6).
+ */


Please indent this comment block

rhenium · 2022-09-14T13:45:59Z

ext/openssl/ossl_ssl.c

+     *   context.keylog_cb = proc do |ary|
+     *     _sock, line = ary
+     *     File.open('ssl_keylog_file', "a") do |f|
+     *       f.write("#{line}\n")
+     *     end
+     *   end


Suggested change

* context.keylog_cb = proc do |ary|

* _sock, line = ary

* File.open('ssl_keylog_file', "a") do |f|

* f.write("#{line}\n")

* end

* end

* context.keylog_cb = proc do |_sock, line|

* File.open("ssl_keylog_file", "a") do |f|

* f.puts(line)

* end

* end

rhenium · 2022-09-14T13:46:39Z

test/openssl/test_ssl.rb

+        assert_not_nil(prefixes.delete(line.split.first))
+      end
+
+      start_server(ignore_listener_error: true) do |port|


ignore_listener_error: true is not needed anymore

cdelafuente-r7 · 2022-09-19T12:58:14Z

Thanks @rhenium for the examples and guidance. I chose the temporary struct, since I believe it is easier to read. I also addressed the other issues. Please, let me know if there is anything else to change. Thanks!

rhenium

We can use a little cleanup (I added an inline comment), but this otherwise looks good to me. Thanks!

rhenium · 2022-09-19T13:20:22Z

ext/openssl/ossl_ssl.c

+    if (state) {
+        rb_ivar_set(ssl_obj, ID_callback_state, INT2NUM(state));
+        return;
+    }


You can let ossl_call_keylog_cb do rb_str_new_cstr() inside it. That way we can remove rb_str_new_cstr_stub() and need just one rb_protect() call.

Absolutely! Thanks for the heads up! I made the changes in 9f99b9e

rhenium · 2022-09-19T14:10:42Z

ext/openssl/ossl_ssl.c

+
+    ssl_obj = (VALUE)SSL_get_ex_data(ssl, ossl_ssl_ex_ptr_idx);
+    args.ssl_obj = ssl_obj;
+    args.line = (VALUE)line;


One last thing, we can turn struct ossl_call_keylog_cb_args.line into a const char * to remove unnecessary casts.

Sure! It makes sense. I changed this in 97d44f8.

- This callback is invoked when TLS key material is generated or received, in order to allow applications to store this keying material for debugging purposes. - It is invoked with an `SSLSocket` and a string containing the key material in the format used by NSS for its SSLKEYLOGFILE debugging output. - This commit adds the Ruby binding `keylog_cb` and the related tests - It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements `SSL_CTX_set_keylog_callback()` from v3.4.2, it does nothing (see libressl/openbsd@648d39f)

rhenium · 2022-09-20T08:13:21Z

Squashed commits

rhenium · 2022-09-20T08:13:55Z

Thank you!

cdelafuente-r7 · 2022-09-20T08:26:37Z

Thank you for landing it!

cdelafuente-r7 commented Aug 29, 2022

View reviewed changes

rhenium reviewed Aug 31, 2022

View reviewed changes

cdelafuente-r7 commented Sep 14, 2022

View reviewed changes

rhenium reviewed Sep 14, 2022

View reviewed changes

rhenium reviewed Sep 19, 2022

View reviewed changes

rhenium force-pushed the add_keylog_cb branch from 97d44f8 to 3b63232 Compare September 20, 2022 08:12

rhenium merged commit 173be66 into ruby:master Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to SSL_CTX_set_keylog_callback() #536

Add support to SSL_CTX_set_keylog_callback() #536

cdelafuente-r7 commented Aug 29, 2022

cdelafuente-r7 Aug 29, 2022

rhenium Aug 31, 2022

rhenium Aug 31, 2022

rhenium Aug 31, 2022

cdelafuente-r7 Sep 14, 2022

rhenium Aug 31, 2022

rhenium Aug 31, 2022

cdelafuente-r7 Sep 14, 2022

rhenium Aug 31, 2022

cdelafuente-r7 Sep 14, 2022

rhenium Aug 31, 2022

cdelafuente-r7 Sep 14, 2022

rhenium commented Aug 31, 2022

cdelafuente-r7 commented Sep 6, 2022

cdelafuente-r7 Sep 14, 2022

rhenium Sep 14, 2022

rhenium Sep 14, 2022

rhenium Sep 14, 2022

cdelafuente-r7 commented Sep 19, 2022

rhenium left a comment

rhenium Sep 19, 2022

cdelafuente-r7 Sep 19, 2022

rhenium Sep 19, 2022

cdelafuente-r7 Sep 19, 2022

rhenium commented Sep 20, 2022

rhenium commented Sep 20, 2022

cdelafuente-r7 commented Sep 20, 2022

Add support to SSL_CTX_set_keylog_callback() #536

Add support to SSL_CTX_set_keylog_callback() #536

Conversation

cdelafuente-r7 commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium commented Aug 31, 2022

cdelafuente-r7 commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdelafuente-r7 commented Sep 19, 2022

rhenium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhenium commented Sep 20, 2022

rhenium commented Sep 20, 2022

cdelafuente-r7 commented Sep 20, 2022